Conversation
|
Thanks for this, @aakhter. This moves the inline rename write out of the editor into a small per-session queue, so a confirmed name survives a reopened or cancelled editor and two quick renames can no longer race. It also fixes the rail editor's layout when a tab has a long Should fix
Small ones
Landing This lands after #525: the group rename test calls What I verified locally
|
The grouped vertical rail can now be edited from the browser: groups are
created, renamed, reordered and deleted, and tabs are moved between them, by
menu, keyboard or pointer drag. Every edit is saved through the existing
PUT /api/tab-layout; there are no server changes.
Saving (tab-layout-browser.js, pure):
- Edits are named operations (createGroup, renameGroup, deleteGroup,
reorderGroup, moveRef) applied to the rail at once, mirroring the server
model: a moved session takes the sessions that still follow it, and a
hand-moved child is marked placement 'manual'. normalizeLayout now keeps
placement and updatedAt, since whole layouts are written back.
- createEditCoordinator keeps ONE PUT {baseVersion, layout} in flight. Edits
made in the same turn share a write; edits made while one is in flight go
out on the version it returns. A 409 replays the operations onto the
layout the server returned and retries (bounded); an operation that no
longer applies is dropped and reported. A 400 re-reads first; any other
failure reports and re-reads.
- dropOperation maps a finished drag to one operation, or null for a drop
that changes nothing.
Wiring (app.js, tab-rail-resize.js):
- The session row menu gains Move up/down, Move to <group>, Move to
Ungrouped and Move to new group in the vertical rail. Before the first
group exists it offers only "Move to new group", which is how a flat rail
becomes grouped; the header strip's menu is unchanged.
- A group header opens its menu with Shift+F10 / ContextMenu, right-click or
a hover glyph (a non-focusable aria-hidden span, so the treeitem still
holds no interactive child): Rename, New group, Move group up/down,
Delete. F2 renames inline. A web tab row's Shift+F10 opens its settings
plus the same moves.
- The menu closes on Escape (consumed before the global Escape handler, focus
back to its row or header), a pointer outside, Tab, focus leaving it, a
resize, a second open and any full re-render.
- Inline group rename shares the session rename's ownership handle, so only
the current editor releases the render guard. Enter or blur commits,
Escape cancels, IME composition keys are left to the IME, and the label
becomes a flex slot so the editor gets the full width while typing.
- Pointer drag (mouse and pen) in the grouped rail only: rows before/after a
row or into a group, a header drag reorders groups. Escape cancels; the
click that ends a drag neither selects nor toggles. The flat rail and the
header strip keep their HTML5 drag untouched.
- A tab:layoutChanged read is deferred while a write is in flight and run
once it settles; a read otherwise rebases unsaved edits. On pagehide,
unconfirmed edits go out in a keepalive PUT and into sessionStorage, and
replay after reload (a no-op when the keepalive landed).
- New strings have zh-CN entries; group names reach the DOM only as text.
Unchanged: the flat rail's markup when no group exists, the tree semantics
and single roving tab stop, sessionOrder and Alt+N.
Tests: test/tab-layout-editing.test.ts (operations, coordinator, drop
mapping, menus, rename, dismissal, SSE deferral, reload recovery, flat-rail
identity) and test/tab-layout-editing.browser.test.ts (real pointer drags,
editor paint, menu Escape), listed in BROWSER_TEST_GLOBS.
- Pointer drag: a press released outside the rail no longer lingers. The
release is heard on window while a press is pending, a move with the
primary button up cancels it, a new press cancels any previous drag, and
an existing Escape listener is removed before another is added, so no
orphaned capture listener can swallow Escape before the terminal.
- Inline group rename: a commit by blur leaves focus where the user put it;
Enter and Escape still return focus to the header.
- A failed layout read while edits are pending keeps the held layout and the
editor and re-reads once the write settles, so a 409 is still rebased.
Dropping unsaved work now always says so in a toast.
- "Move to <group>" quotes the group name (with a matching zh-CN pattern), so
a group named "New group" or "ungrouped" no longer reads or translates like
the fixed entries.
- The group menu glyph stays visible under (hover: none).
- The sessionStorage replay copy carries { owner, baseVersion, savedAt } and is
ignored for another owner, after 60 s, or against an older layout. A move
with no anchor carries no index, so a replay keeps the row last.
- A 400 that survives the re-read is reported as "Could not save tab groups."
- closeTabRailActionMenu() no longer removes the group menu's DOM.
- Cancelling "Delete group" returns focus to the header.
- Stale comments updated.
Two problems with renaming a tab in the vertical rail, both easier to hit now that the grouped rail has its own inline editor beside the session one. Writes. A committed rename PUT its name and only applied the answer if the same editor was still open when it came back. Reopening the editor before the PUT answered (F2 or right-click again, or starting a group rename, which cancels the session editor) threw the confirmed name away, so the tab kept showing the old name until an SSE frame happened to repaint it. Two quick renames also raced as two concurrent PUTs. Inline renames now go through a per-session queue: one PUT at a time in the order they were made, the confirmed name applied to app.sessions whatever happened to the editor, and the "already that name" check made when the write runs rather than when Enter is pressed, so confirming the name still on screen over a write in flight is a real write. Layout. The editor (a flex row) could not shrink below the input's intrinsic width, so a long w<n>-<case> prefix pushed the label past its row: the prefix slid out of view in the detailed rows and the input was clipped mid-word in the compact rail. The label now has min-width 0, the prefix gives way first (down to 2rem, with an ellipsis), the input keeps 4rem, and in the compact rail the row's adornments step aside while the name is edited. The detailed rows' three-line clamp also outranked the shared unclamp rule, which is what the existing "unclamped editor" browser test caught; it is restated there. Header strip, sidebar and flat-rail markup are unchanged. Tests (test/inline-rename.test.ts, browser suite): the unclamp check runs for simple and detailed rows; a write-ordering describe covers ordering, a reopened editor cancelled over a confirmed write, a re-sent unchanged name and a group rename taking over; a long-prefix describe drives real rows from a live session in simple, detailed and compact rails.
Reopening the editor over a rename still in flight filled it from the name the server had not replaced yet, so dismissing it (blur commits) queued the old name behind the new one and undid the rename. The queue now records the newest queued name per session (_inlineRenamePending, cleared with the queue entry), and a reopened editor takes its prefix, input and "unchanged" comparison from it. An untouched confirm sends nothing more. A failed write only toasted while its editor was still current. The queue reports the failure itself now, and the editor only puts its label back. One rejected task blocked every later rename of that session until reload. Each task now chains from a settled predecessor, the local apply after a successful PUT is guarded, and the queue entry is cleaned up on either outcome. The rail and sidebar editor's 4rem floor moves from a stylesheet `!important` into the inline min-width startInlineRename already writes per layout (0 in the header strip, 4rem in the rail and sidebar). Tests: the reopened-editor case now expects only "First" to be sent; new cases cover a 500 answered after the editor is gone and a throw in updateSubagentParentNames; the long-prefix check runs in the sidebar and detailed sidebar too and asserts the inline floor; the header strip editor keeps min-width 0.
|
Thanks for the careful review, and for reproducing each one with held PUTs; that made them easy to pin down. All five are addressed in a follow-up commit on this branch:
Each new or changed test failed before its fix, and |
4b50d52 to
243797e
Compare
|
Thanks @aakhter. I checked 243797e against all five items and everything is in. The rebased a61bbbf is patch-identical to the commit I reviewed ( What I checked
One optional nit: reopen over a rename in flight and confirm it unchanged, and the label shows the old server name ("Old") until the held PUT lands ( Ready to merge after #525. |
2 participants
Stacked on #525 (#517 and #519 are now in master). Only the last two commits are new: the fix and its review follow-up.
This fixes two problems with renaming a tab inline in the vertical rail. Both are easier to hit now that the grouped rail has its own group editor next to the session one.
Rename writes. A committed rename only applied the server's answer if the same editor was still open when the PUT returned. Reopening the editor before then (F2 or right-click again, or starting a group rename, which cancels the session editor) threw the confirmed name away. The tab showed the old name until an SSE frame happened to repaint it, and two quick renames raced as two concurrent PUTs.
Inline renames now go through a per-session queue:
app.sessionswhatever happened to the editor.Editor layout. The editor could not shrink below the input's intrinsic width. A long
w<n>-<case>prefix pushed the label past its row: the prefix slid out of view in the detailed rows, and the input was clipped mid-word in the compact rail. Now:The detailed rows' three-line clamp also outranked the shared unclamp rule, which is why the existing "unclamped editor ... restores clamp on cancel" browser test failed on master. That rule is now restated for the detailed rows, and the test passes.
The layout fix applies to both vertical layouts, the tab rail and the session sidebar, since they share these rules: the input's 4rem floor is the editor's own inline min-width, chosen per layout (0 in the header strip, 4rem in the rail and sidebar). The sidebar's markup is unchanged but its editor gets the same fix, and the long-prefix test covers the simple, detailed and compact rail plus the sidebar and the detailed sidebar. The header strip is pixel-identical, and there are no server changes. Tests are in
test/inline-rename.test.ts(browser suite, already inBROWSER_TEST_GLOBS): the vertical test now runs for both simple and detailed rows, plus new write-ordering and long-prefix cases. 8 of its 22 tests fail on the parent commit.Touch drag in the grouped rail is still not included. Upstream has no drag handle to hang it on, so it would need a new handle design first.